Still having trouble figuring out why TestGraphsyncRoundTripMultipleAlternatePersistence uniquely doesn't run. It's to do with the blockstore though, an error comes out of the LoadAttemptQueue on ClearRequest: "No active request" which halts collection of the chain. So something about having multiple persistence stores is getting borked? Somehow the requestid is mixed up this. There's various places where we index by requestid: map[graphsync.RequestID]... but switching from an int to a uuid should bother that. Unless we had overlapping requestids somehow before and now we have guaranteed uniqueness?

I figured out the TestGraphsyncRoundTripMultipleAlternatePersistence problem, although I'm not really sure why this was the only test that was failing but I think it's something to do with using type RequestID uuid.UUID as a map key since uuid.UUID is type UUID []byte and slices aren't comparable, or something which causes problems for map keys. I wonder if perhaps we're dealing with identity comparison in most cases but TestGraphsyncRoundTripMultipleAlternatePersistence is hitting comparison rather than identity checking ..?

Anyway, by switching it out to type RequestID string and using it to hold bytes (not the string form, you have to .String() it for that), then it becomes comparable and safer to use all over the place. It's also neat because it's shown up other type errors that using uuid.UUID didn't—a bunch of %d in S?printf() calls which were apparently fine with uuid.UUID for some reason, but not with string.

So aside from the network message version switching, this is working as it should I believe.

@hannahhoward see latest commit for my first pass at protocol switching and compatibility: f7ce568

No tests yet that exercise v1.0.0.

Pushed another commit that adds testing, but failing testing. Fixed a few other things along the way. But I think the main problem is the lack of differentiation between the integer ID space for requests and responses. So I need to figure out a nice way to do that.

so baseline: this looks reasonable. The MessageHandler is a good abstraction. But two caveats, one big, one small:

Big: To uniquely identify the OLD requests you need the peer ID && the integer request ID. So, for your MessageHandler, you need the following maps:

type oldRequestKey struct {
   p peer.ID
   requestID in32
}
// ...
type MessageHandler struct {
  // ...
  fromV1Map map[oldRequestKey]graphsync.RequestID
  toV1Map   map[graphsync.RequestID]oldRequestKey
  //...
}

The translation is:

• when I send out a message, if I'm writing to a v1 client:
• requests get encoded as (other peer, integerRequest)
• responses get encoded as (my peer, integerRequests)

Beyond distinguishing requests & responses, if we are talking to old client A & old client B, it's entirely possible both could send you a request with the same integer ID.

2. Personally I would just write two different versions of newMessageFromProto rather than have the pbMessage abstraction. Especially since that's generated code and it's likely to change with the IPLD version

also, in terms of where to get the other peer.ID and the self peer.ID:
LibP2P Host.ID has the self peer
When you call NewMessageSender or SendMessage you have the peer.ID of the other peer

sidebar: goodness why did I decide to have so much object orientation initially? I wonder if this would be made much easier if we took all this encoding out of the message package and just made and encode/decode package with functional implementations

This is more of a "later problem" than a now problem though so let's defer it.

OK, this is working and ready for review. But it's blocking pending a merge with #323 so that v1.1.0 isn't protobuf but is dag-cbor.

#332 has these commits and more now, it removes the 1.1.0 protocol, adding a 2.0.0 that's dag-cbor with UUIDs and retains the translation layer that was introduced here for 1.1.0 / 1.0.0.